NEW: Support entering playmode with Domain Reload disabled ("Faster Enter Playmode")#2343
NEW: Support entering playmode with Domain Reload disabled ("Faster Enter Playmode")#2343
Conversation
- Rework InputSystem's Init functions to handle optional domain reloads - Bypass InitializeInPlayer() "Reset" and "Restore" functions if DRs disabled Only the first step to getting this working, but enables FEPM support for golden path scenarios.
- Move Reset/Restore state functionality out of InputSystem to the Test assembly (InputTestStateManager.cs)
- Refactor InputManager Init/Dispose to be cleaner and better abstracted:
* Adds CreateAndInitialize static method
* Replaces Destroy() with IDisposable implementation
* InputManager creates "default" InputSettings object if none provided
* Runtime, Settings, and Metrics fields now private
- Update InitializeInEditor() to incorporate changes
- Update and fix tests
For the most part, the logic should be mostly preserved. InitializeInEditor() has the biggest (logical)
change because Reset() (moved to Tests) contained some actual init calls that needed to be pulled out.
However, we *should* be making the same calls in the same order.
However, this change does seem to "break" some of the OnScreenTests(); they're now unstable.
This will need to be fixed in a later commit.
- Rework the class so it operates as a Singleton (basically how it's being used)
- Move actions get/set implementations from InputSystem to ProjectWideActions class
* Combine functionality for tighter cohesion despite being an Editor
* Update #ifefs to allow Player to access some of these members
- Call EnsureInitialized() from Editor init flows to fix the actual bug
Tested using ProjectWideActions sample and verified could move cube when domain reloads disabled.
Ran Package tests (Edit/Play/Player) to verify no (new) failures.
…ns (ISX-1842) - Make InputSystem static fields private with read-only accessors - Add InputSystemTestHooks.cs for "Test Hook" operations that need direct access to fields - Factor out InputSystemObject (and related State) and rename to InputSystemStateManager - Factor out "dirty asset tracking" functionality to a separate Utility class Despite touching a bunch of files, this is a low-risk set of changes; functions are moved to new files and types renamed, but no real changes to functionality. Validated changes with Edit/Play/Player tests.
- Most ProjectWideAction changes discarded since feature was re-implemented (no longer relevant) - Recreate InputSystem "Test Hook" for Enabling/Disabling Actions from tests - Small fixes from unresolved conflicts or bad merges during rebase - Various formatting changes (to be more consistant)
- Consolidate functionality into DeferBindingResolutionContext class - Instantiate Context object as a non-static InputManager field exposed via InputManager methods
- Static field (in each class) initialized via RuntimeInitializeOnLoadMethod() hook - This functionality is a bit clunky and should be refactored but that's out-of-scope for this work
- Add a flag to guard against re-initializing Plugins within PerformDefaultPluginInitialization() - Refactor SteamSupport to fix Init/Shutdown flows and improve state management - Refactor PlayerInput's static fields into a single GlobalState struct (matching other classes) - Move EnhancedTouch static fields into Touch's GlobalState struct - Minor refactoring of SteamSupport to improve init/cleanp-up flows (especially with tests)
…ts (ISX-1840) - Consolidate Touchscreen's cached settings into a separate struct - Rework NativeInputRuntime initialization to (fully) employ Singleton pattern - Refactor Actions_CanHandleModification TestCase generator to work without Domain Reloads - Fix Device static fields not getting reset during SimulateDomainReload()
- Rename some variables - Add update some comments - Other small tweaks.
# Conflicts: # Packages/com.unity.inputsystem/InputSystem/Actions/InputActionMap.cs # Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs # Packages/com.unity.inputsystem/InputSystem/InputAnalytics.cs # Packages/com.unity.inputsystem/InputSystem/InputManager.cs # Packages/com.unity.inputsystem/InputSystem/InputSystem.cs # Packages/com.unity.inputsystem/Tests/TestFixture/InputTestFixture.cs
…invalid formatting
…d from a serialisation thread and force create ScriptableObjects
K-Tone
changed the title
…er playmode anymore
|
Hey Hakan! Here is my branch that is based on yours really, with just a few minor corrections. Let's take a look at it together and define what are the larger bits of work that are still missing. |
|
@u-pr See below all the 87 issues that Unity's Project Auditor has found regarding the domain reload in this branch. Can you analyse them and see if any are still legit? I mean that some issues are reported, but the check itself is somewhat formal and doesn't appear to take into account that statics of a class can be initialised from other classes, not necessarily from the dedicated methods of the class. {"ignored":false,"descriptorId":"UDR0001","category":"DomainReload","properties":["UDR0001","Unity.InputSystem"],"description":"No method with RuntimeInitializeOnLoadMethod attribute","location":{"IsValid":true,"line":207,"path":"Packages/com.unity.inputsystem/InputSystem/Devices/Sensor.cs"},"severity":"Moderate"}, |
|
I've analyzed the 87 issues reported by the Project Auditor in the context of this PR. Your intuition is correct: many of these are "formal" hits where the auditor's static analysis is too shallow to recognize the consolidated initialization pattern introduced in this branch. Key Findings
RecommendationMost of these hits can be safely ignored as "formal" issues. However, it might be worth adding explicit nullification for the 🤖 Helpful? 👍/👎 |
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
|
/improve |
|
Persistent review updated to latest commit f38866b |
There was a problem hiding this comment.
Tested: Parts of the AI test plan here on github, the pre-production test plan, ran all of our sample scenes in all FEPM modes, ran the survival kids project in all modes looking for broken inputs. Checked performance via memory profiler by entering and leaving play mode a lot with reloads disabled, usage did not increase. (Windows and Mac)
If you have any other ideas that need testing let me know
Packages/com.unity.inputsystem/InputSystem/Actions/InputActionRebindingExtensions.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Actions/InputActionRebindingExtensions.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Actions/InputActionRebindingExtensions.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Actions/InputActionRebindingExtensions.cs
Outdated
Show resolved
Hide resolved
MorganHoarau
left a comment
MorganHoarau
left a comment
There was a problem hiding this comment.
Didn't found anything major. I didn't review all the tests but I'm trusting the process.
Sharing my notes in case we need to come back to the PR in the future.
Packages/com.unity.inputsystem/InputSystem/Actions/InputActionRebindingExtensions.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Devices/Touchscreen.cs
Outdated
Show resolved
Hide resolved
Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs
Show resolved
Hide resolved
|
@K-Tone Seems like a formatter run would be necessary. Also noticed there are many unresolved comments in this review from peers and bots that might be good to review with regards to what will be acted on (or not). |
Co-authored-by: Morgan Hoarau <122548697+MorganHoarau@users.noreply.github.com>
Co-authored-by: Morgan Hoarau <122548697+MorganHoarau@users.noreply.github.com>
|
@u-pr Can you analyse comments on this PR and see which ones still need reacting to? |
|
I've analyzed the comments and compared them with the current state of the PR. While many points have been addressed, there are several items that appear to be pending a reaction or implementation: 🛠️ Code Cleanup & Quality
🏗️ Refactoring & Consistency
❓ Pending Clarifications
✅ Resolved ItemsFor context, the following were verified as addressed:
🤖 Helpful? 👍/👎 |
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
Went through this again and it looks very good @K-Tone! Excellent job. I noticed @MorganHoarau had a few comments that remain open and I had a previous comment which I left unresolved on why we didn't address the multiple statics in Steam support.
Also ran some samples again with domain reloads disabled but didn't run into any problems.
See no need to block this PR so I'll approve now.
| /// The s_ConnectedControllers array is allocated in response to setting event handlers and | ||
| /// so it can double as our "is installed" flag. | ||
| /// </remarks> | ||
| private static bool updateHooksInstalled => s_ConnectedControllers != null; |
There was a problem hiding this comment.
Seems like this wasn't acted upon? Any reason why?
jfreire-unity
left a comment
jfreire-unity
left a comment
There was a problem hiding this comment.
I don't want to block this PR and since so many people have tested it, I'm confident it works. I only raised a question to see if I understand the intent of the change vs the original branch.
Great work @K-Tone , it's great to have this finally in the package.
…/PlayerInput.cs Co-authored-by: Morgan Hoarau <122548697+MorganHoarau@users.noreply.github.com>
7 participants
Description
This branch aims to enable entering the play mode with Domain Reload disabled, it's also called Faster Enter Playmode sometimes.
The change is a bit non-trivial because we seem to have quite a bunch of statics, and all of them have to be reset the right way when Domain Reload is disabled.
The branch is basically a straightforward merge of the current develop branch into Hakan's #2016 branch, which in turn is based on Tim's incredible original branch: #1915.
Testing status & QA
Tested by Paulius and Hakan, local testing on Anthony's machine as well.
Overall Product Risks
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.